api: add host normalization to ClientTrafficPolicy - #9115
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc4204524e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9115 +/- ##
==========================================
+ Coverage 75.47% 75.48% +0.01%
==========================================
Files 252 252
Lines 41662 41667 +5
==========================================
+ Hits 31445 31453 +8
+ Misses 8087 8086 -1
+ Partials 2130 2128 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/retest |
|
cc @arkodg for another pair of eyes on the API. |
There was a problem hiding this comment.
Pull request overview
Adds first-class Host/Authority header normalization knobs to ClientTrafficPolicy (parallel to existing path normalization) and wires them through IR and xDS so users no longer need fragile EnvoyPatchPolicy patches for these Envoy HttpConnectionManager options.
Changes:
- API: introduce
spec.hostwithstripPortandstripTrailingHostDotfields and regenerate CRDs/docs/deepcopy. - Implementation: translate
ClientTrafficPolicy.spec.host→ IR (ir.HTTPListener.Host) → xDS HCM (strip_any_host_portoneof +strip_trailing_host_dot). - Tests/docs: add gatewayapi + xDS translator golden coverage, task doc, and a release note entry.
Reviewed changes
Copilot reviewed 18 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
api/v1alpha1/clienttrafficpolicy_types.go |
Adds spec.host to ClientTrafficPolicySpec. |
api/v1alpha1/hostsettings_types.go |
Introduces the HostSettings API type and field documentation. |
api/v1alpha1/zz_generated.deepcopy.go |
Regenerates deepcopy logic for the new API type/field. |
internal/gatewayapi/clienttrafficpolicy.go |
Translates spec.host into ir.HTTPListener.Host (with empty/no-op handling). |
internal/gatewayapi/testdata/clienttrafficpolicy-host-strip-port.in.yaml |
GatewayAPI golden input for host.stripPort. |
internal/gatewayapi/testdata/clienttrafficpolicy-host-strip-port.out.yaml |
GatewayAPI golden expected output for host.stripPort. |
internal/gatewayapi/testdata/clienttrafficpolicy-host-strip-trailing-dot.in.yaml |
GatewayAPI golden input for host.stripTrailingHostDot. |
internal/gatewayapi/testdata/clienttrafficpolicy-host-strip-trailing-dot.out.yaml |
GatewayAPI golden expected output for host.stripTrailingHostDot. |
internal/gatewayapi/testdata/clienttrafficpolicy-host-empty.in.yaml |
GatewayAPI golden input validating host: {} is a no-op. |
internal/gatewayapi/testdata/clienttrafficpolicy-host-empty.out.yaml |
GatewayAPI golden expected output for the host: {} no-op case. |
internal/ir/xds.go |
Adds Host *HostSettings to ir.HTTPListener and defines IR HostSettings. |
internal/ir/zz_generated.deepcopy.go |
Regenerates IR deepcopy logic for the new HostSettings and listener field. |
internal/xds/translator/listener.go |
Emits HCM host normalization fields (strip_any_host_port + strip_trailing_host_dot) from IR. |
internal/xds/translator/testdata/in/xds-ir/host-normalization.yaml |
New xDS translator golden input covering host normalization permutations. |
internal/xds/translator/testdata/out/xds-ir/host-normalization.listeners.yaml |
Expected listener/HCM output including host normalization fields. |
internal/xds/translator/testdata/out/xds-ir/host-normalization.routes.yaml |
Expected routes output for the golden scenario. |
internal/xds/translator/testdata/out/xds-ir/host-normalization.clusters.yaml |
Expected clusters output for the golden scenario. |
internal/xds/translator/testdata/out/xds-ir/host-normalization.endpoints.yaml |
Expected endpoints output for the golden scenario. |
charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml |
Regenerated CRD schema including spec.host. |
charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml |
Regenerated CRD Helm template including spec.host. |
test/helm/gateway-crds-helm/all.out.yaml |
Updated Helm golden output including the new CRD schema. |
test/helm/gateway-crds-helm/e2e.out.yaml |
Updated Helm golden output including the new CRD schema. |
test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml |
Updated Helm golden output including the new CRD schema. |
site/content/en/latest/api/extension_types.md |
Updates rendered API reference to include HostSettings and spec.host. |
site/content/en/latest/tasks/traffic/host-header-normalization.md |
Adds a task guide for configuring host normalization. |
release-notes/current.yaml |
Adds a release note entry under “new features”. |
Files not reviewed (2)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
- internal/ir/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@sboulkour can you rebase with main and redo the release notes after #9312? |
47bed87 to
fb1b4ff
Compare
@zirain Rebase done. |
|
@zirain Shall I wait for a second approval to merge ? Not certain about the process here. Also, could you re-run tests please |
|
/retest |
|
@zirain should I rebase and merge ? |
Address Copilot review on envoyproxy#9115: Envoy's strip_any_host_port not only strips the port for route matching but also normalizes the actual Host/:authority value forwarded upstream. Make the StripPort godoc state this explicitly (mirroring StripTrailingHostDot's upstream-header note), so the generated CRD/API docs reflect the real effect. Regenerated CRDs, helm snapshots and API docs. Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
|
|
||
| // Host enables managing how the Host/Authority header set by clients can be normalized. | ||
| // | ||
| // +optional | ||
| Host *HostSettings `json:"host,omitempty"` |
7ff6c80 to
fab6a88
Compare
…olicy Per maintainer review on envoyproxy#9115, relocate the host normalization settings (stripPort, stripTrailingHostDot) from the top-level spec.host into spec.headers.host, since the Host/Authority header is a header concern and belongs alongside the other HeaderSettings. The IR representation (HTTPListener.Host) is unchanged; only the CTP API surface and the CTP->IR translation source path move. As a result, setting host normalization now makes spec.headers non-nil, so the listener header defaults (withUnderscoresAction: RejectRequest, already Envoy's default) appear in the IR for the host-only fixtures. Regenerated deepcopy, CRDs, helm snapshots, API docs and golden testdata, and updated the host-header-normalization task doc and release note. Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Address Copilot review on envoyproxy#9115: Envoy's strip_any_host_port not only strips the port for route matching but also normalizes the actual Host/:authority value forwarded upstream. Make the StripPort godoc state this explicitly (mirroring StripTrailingHostDot's upstream-header note), so the generated CRD/API docs reflect the real effect. Regenerated CRDs, helm snapshots and API docs. Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
|
/retest |
fab6a88 to
9ad6f4b
Compare
…olicy Per maintainer review on envoyproxy#9115, relocate the host normalization settings (stripPort, stripTrailingHostDot) from the top-level spec.host into spec.headers.host, since the Host/Authority header is a header concern and belongs alongside the other HeaderSettings. The IR representation (HTTPListener.Host) is unchanged; only the CTP API surface and the CTP->IR translation source path move. As a result, setting host normalization now makes spec.headers non-nil, so the listener header defaults (withUnderscoresAction: RejectRequest, already Envoy's default) appear in the IR for the host-only fixtures. Regenerated deepcopy, CRDs, helm snapshots, API docs and golden testdata, and updated the host-header-normalization task doc and release note. Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Address Copilot review on envoyproxy#9115: Envoy's strip_any_host_port not only strips the port for route matching but also normalizes the actual Host/:authority value forwarded upstream. Make the StripPort godoc state this explicitly (mirroring StripTrailingHostDot's upstream-header note), so the generated CRD/API docs reflect the real effect. Regenerated CRDs, helm snapshots and API docs. Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Add a new `host` section to `ClientTrafficPolicy` to normalize the Host/Authority header on the listener, parallel to the existing `path` section: - `host.stripTrailingHostDot` maps to Envoy `strip_trailing_host_dot`, so requests with `Host: example.com.` match routes for `example.com` (NGINX-parity behavior). - `host.stripPortMode` (`Any`/`Matching`) maps to Envoy `strip_any_host_port` / `strip_matching_host_port`. Both were previously only achievable via fragile EnvoyPatchPolicy. This change is split out of envoyproxy#8825 per maintainer review, which scoped that PR to `maxRequestHeaderBytes`. Fixes envoyproxy#8832 Co-authored-by: albsga4 <asalvador@newrelic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Drop the stripPortMode (Any|Matching) enum on ClientTrafficPolicy's host section and replace it with a stripPort *bool that maps to Envoy's strip_any_host_port (unconditional stripping). The Matching mode mapped to strip_matching_host_port, which compares the Host header port against the Envoy listener port. Since a Gateway listener on e.g. port 80 is translated to an Envoy listener on port 10080, Matching never strips the port clients actually use, so it was silently ineffective. As discussed in the PR review, only unconditional stripping is exposed, expressed as a boolean per maintainer suggestion. Regenerated CRDs, deepcopy, helm snapshots, docs and golden testdata, and renamed the strip-port-matching gatewayapi test to strip-port. The release note is now a fragment under release-notes/current/new_features/ following the split-release-notes workflow (envoyproxy#9312). Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
…olicy Per maintainer review on envoyproxy#9115, relocate the host normalization settings (stripPort, stripTrailingHostDot) from the top-level spec.host into spec.headers.host, since the Host/Authority header is a header concern and belongs alongside the other HeaderSettings. The IR representation (HTTPListener.Host) is unchanged; only the CTP API surface and the CTP->IR translation source path move. As a result, setting host normalization now makes spec.headers non-nil, so the listener header defaults (withUnderscoresAction: RejectRequest, already Envoy's default) appear in the IR for the host-only fixtures. Regenerated deepcopy, CRDs, helm snapshots, API docs and golden testdata, and updated the host-header-normalization task doc and release note. Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Salim B <9945903+sboulkour@users.noreply.github.com>
Address Copilot review on envoyproxy#9115: Envoy's strip_any_host_port not only strips the port for route matching but also normalizes the actual Host/:authority value forwarded upstream. Make the StripPort godoc state this explicitly (mirroring StripTrailingHostDot's upstream-header note), so the generated CRD/API docs reflect the real effect. Regenerated CRDs, helm snapshots and API docs. Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Scope the first iteration of host normalization down to trailing-dot removal only, removing the StripPort field (Envoy's strip_any_host_port) per maintainer feedback. Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
- docs: qualify the field reference as spec.headers.host.stripTrailingHostDot (review feedback endorsed by @arkodg) - test: drop the now-redundant third xDS listener; keep one listener enabling stripTrailingHostDot and one control with no host settings Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
…Settings Handle headers.host alongside the other HeaderSettings sub-fields instead of through a separate helper and call site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
The field is nil-checked just above, so dereference it directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Sends a request with a trailing dot in the Host header through a listener with ClientTrafficPolicy headers.host.stripTrailingHostDot enabled, and verifies the route matches and the backend receives the normalized Host header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
9ad6f4b to
4bebf55
Compare
|
/retest |
1 similar comment
|
/retest |
What type of PR is this?
api: add ahostsection underClientTrafficPolicy'sheadersfor Host/Authority header normalization.What this PR does / why we need it:
Adds a new
hostsection underClientTrafficPolicy'sheaders, to normalize the Host/Authority header on the listener. A setting that previously required a fragileEnvoyPatchPolicyis now a native field:spec.headers.host.stripTrailingHostDot→ Envoystrip_trailing_host_dot. Envoy does not strip the trailing dot by default (unlike NGINX), soHost: example.com.fails to match routes forexample.com. When a port is present (example.com.:443), only the trailing dot of the host is stripped, leaving the port as-is (example.com:443). This affects the upstream Host header as well.This is split out of #8825 per maintainer review (@arkodg), which scoped that PR to
maxRequestHeaderBytes. The host-normalization work is co-authored with @albsga4.Includes: API types, IR, gatewayapi + xDS translation, regenerated CRDs/deepcopy/docs, gatewayapi golden tests (trailing-dot and empty no-op), an xDS translator golden test, an e2e test (trailing-dot host matches the route and is normalized upstream), a task guide, and a release note.
Which issue(s) this PR fixes:
Fixes #8832
Related to #8825
Release Notes: Yes